-
Notifications
You must be signed in to change notification settings - Fork 730
[Needs improvement] Playlist dialog: Add checkmark to indicate if song is in playlist #432
base: master
Are you sure you want to change the base?
Conversation
The "Add to playlist" dialog will show checkmark behind a playlist if the song is already part of that playlist. This feature will only be shown if the playlist dialog is opened and only one song is selected.
I think this won't perform well on larger playlists, since it's doing an new SQL query for every single item. |
@kabouzeid Yes, there might be a problem with very big playlists. But I just tested it on my Android device. My biggest playlist contains all of my 1663 tracks and I can't see any difference between the normal app and my fork version. |
@kabouzeid Do you plan to include this feature? I would love to hear that because think there is no reason not to. The delay may be a drawback. But if the smartphone needs noticeable time to check whether a song is already in a playlist, imagine how long a human would need... |
An alternative solution would be to allow the user to prune a playlist of duplicates, so it would just be a query within a playlist to check for duplicate IDs. |
@Sogolumbo Sorry for the delay, I have very little time for this project currently. Also I don't have any large playlist to test against. Could you measure the time in millis on your 1663 playlist how long the for loop actually takes? |
Instead of displaying a checkmark I would prefer to popup a dialog, in case the playlist already contains at least one song and allow to exclude those songs from adding again to the playlist. |
@arkon this could be added either ways |
I just tested how long the for loop takes. Here are the results:
@kabouzeid What do you mean with "on your 1663 playlist"? In the test environment (my phone) there are 7 playlists. The longest one contains 1663 songs. The other playlists are smaller (180, 3, 253, 188, 22, 91 songs) (all playlists added up: 2400 songs). Device: honor 6C |
By adding a breakpoint the time needed for checking the playlists for the song can be accessed.
@kabouzeid I think that is the right solution if the user tries to add multiple songs / a playlist / an album to other playlists. For just one song I prefer my solution because it creates a faster workflow (for the user). |
@Sogolumbo I apologize, I just notice that I've completely misunderstood your code before. Somehow I thought you would make an SQL query for every song in the playlist. Obviously this is not the case at all. Now I'm even more surprised that this takes 90ms. |
Also if we display the checkmark additionally when the user tries to add exactly one song, it should be a proper material one. In this case we have to add an image view to the items where we display the checkmark. https://material.io/icons/#ic_done |
Yes a material design icon would look better! Wouldn't it be even better if the icon is on the right end? What do you think @kabouzeid? |
@touficbatache absolutely |
@kabouzeid How can we add lyrics to music? |
@touficbatache You could add lyrics to tags or add *.LRC file to the same folder as music file location. |
Thanks!
|
Add Upstream Commits
Merge Updates from kabouzeid/master.
At the end of the playlist name a checkmark is displayed if all songs are in the playlist. If only some of the songs are in the playlist, the checkmark is in brackets.
When adding songs to a playlist duplicates will be filtered out.
Any update or is the fork the best we're going to get? |
@dronewizard Well the authors of the Phonograph seem to care a lot about the design of the app. Unfortunately I have absolutely no Idea how the material design works and I don't have the time learn it. |
I'll look into it. |
No ascii checkmark please. The checkmark should be an image (ic_done) and right aligned. |
Can I highjack the thread for a sec? How do you test the app? I want to make some changes for a PR but I don't know how to make a reasonable testing environment. You know, so I can change some code, and check out how it works on the fly. |
Changed and added functions in PlaylistUtil to improve the efficiency of the AddToPlaylistDialog. It will only be checked once which song is in which playlist. This information is also used to make sure no songs will be added twice to the same playlist (instead of checking again).
Fixed issue #4.
doPlaylistContainsAllSongs returned true if the playlist was empty. That's a wrong result, hence fixed with this commit.
First Prototype: Working check box menu. Only two state-check-boxes (All or nothing)
Removed "Add Playlist" from the checkbox area and transformed it into a button.
The cancel button is removed as it takes up a lot of space and is not strictly necessary. (The same action can be performed by tapping next to the dialog.)
The "Add to playlist" dialog will show a checkmark behind a playlist if the song is already part of that playlist.
This feature will only be shown if only one song is selected for addition.
In the following screenshot the song is already part of the "Example playlist":